-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor the ASTBuilder to get rid of the currentAttr attribute. #585
Conversation
This attribute was used to attach the right docstring node to the right Attribute object. Now it uses AST node navigation (with the .parent attribute) instead for fetching the docstring node for an ast.Assign. This change might not be worth it, on the one hand it removes a attribute beeing mutated at different palces in the code, but replaces this kind of "unsafe" state tracking (meaning not with pop() and push()) by some more verbose solution that involves adding the .parent attribute on all nodes. The zopeinferface extension needed to be adjusted as well because it relied on the docstring assigment feature in an implicit way, now it's explicit what we're doing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #585 +/- ##
==========================================
- Coverage 92.07% 92.03% -0.04%
==========================================
Files 47 47
Lines 8412 8465 +53
Branches 1856 1868 +12
==========================================
+ Hits 7745 7791 +46
- Misses 391 395 +4
- Partials 276 279 +3 ☔ View full report in Codecov by Sentry. |
After a second though, I believe it’s worth it. Mainly because:
|
@tristanlatr sorry to punt on this, but can you address the conflicts and then re-request from @twisted/twisted-contributors rather than me personally? |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…docstring assignments or irgnore it.
…/pydoctor into experimental-refactor-astbuilder
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This PR is ready to be merged. It does not worth a changelog entry since it's only refactoring. It also fixes a edge case bug that probably got unnoticed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Diff from pydoctor_primer, showing the effect of this PR on open source code: numpy (https://github.com/numpy/numpy)
- /projects/numpy/numpy/distutils/ccompiler_opt.py:1038: Existing docstring at line 949 is overriden
- /projects/numpy/numpy/distutils/ccompiler_opt.py:1075: Cannot find link target for "bool"
+ /projects/numpy/numpy/distutils/ccompiler_opt.py:986: Cannot find link target for "bool"
|
Refactor the ASTBuilder to get rid of the currentAttr attribute.
This attribute was used to attach the right docstring node to the right Attribute object. Now it uses AST node navigation (with the .parent attribute) instead for fetching the docstring node for an ast.Assign.
This change removes a attribute being mutated at different places in the code, replaces this kind of "unsafe" state tracking (meaning not with pop() and push()) by some more explicit solution.
The zopeinferface extension needed to be adjusted as well because it relied on the docstring assignment feature in an implicit way, now it's explicit what we're doing.